-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tweak xhr error handling #197
Conversation
Looks good, but I think needs @paddybyers's input for this one |
err.code = 80000; | ||
self.complete(err); | ||
var errorHandler = function(errorEvent, message, code, statusCode) { | ||
var errorMessage = message + ", errorEvent was " + Utils.inspect(errorEvent) + ", current statusText is " + self.xhr.statusText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single quotes please everywhere.
Also, have we checked that Utils.inspect()
gives us something meaningful for the event? It's just JSON.stringify()
so will it expose relevant information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, have we checked that Utils.inspect() gives us something meaningful for the event?
In my tests (just aborting or cutting the connection) it hasn't contained anything useful, no (the stringify was only {isTrusted: true}
, and the rest of the event object didn't have anything better). Left it in there in the hope that maybe there's some more interesting error might happen in the wild where (a) the event object included some new fields, and (b) they'd be included in the stringify. Perhaps not very likely, but I figure leaving it there doesn't cost much. Want me to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I figure leaving it there doesn't cost much. Want me to remove it?
Looking at the event specs you're not going to get anything useful from the event object itself except the type of the event (ie error or abort). TBH I' remove the inspect()
and just have 'type = ' + ev.type
or something like that in the log message.
79a3755
to
96e8ad2
Compare
Thanks |
Not sure whether bundling the errorEvent and the current statusText in there will actually be useful, but, shrug